Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce WildFly module #128

Merged
merged 3 commits into from
Jan 10, 2020
Merged

Conversation

rieckpil
Copy link
Contributor

@rieckpil rieckpil commented Jan 8, 2020

Can you have a first look @aguibert ? I wasn't sure about the license text in the WildFlyAdapter and if you want all changes in one commit or smaller commits.

Furthermore what formatting options do you use? I didn't auto-format with IntelliJ as it was different to yours (maybe introduce a .editorconfig file?).

Signed-off-by: Philip Riecks [email protected]

Signed-off-by: Philip Riecks <[email protected]>
@aguibert aguibert added the is:enhancement New feature or request label Jan 8, 2020
Copy link
Member

@aguibert aguibert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution @rieckpil! Changes look good, just one question about the health endpoint. Copyright can stay the way it is or you can change it to your name instead of IBM Corporation if you want


@Override
public Optional<String> getReadinessPath() {
return Optional.of("/health");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you know if the /health endpoint is always enabled in wildfly 18.0.1.Final? Also, does it seem so be pretty consistent that the /health endpoint only returns HTTP 200 when the app is ready to serve requests?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for this hint. I checked it and in the documentation they say it's included in the standalone configurations included in the WildFly distribution. But while searching I found that the /health endpoint is served from the management port of WildFly (9990 by default). I think this won't work with the current setup. I could think of introducing a management port attribute within ServerAdapter or adjusting getReadinessPath() to return a URL ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's interesting that the documentation says the module is enabled "standalone" deployments, but then it goes on to describe how to add the module manually, which implies that there can be images of Wildfly that do not have the health module enabled?
Just to be safe lets return an empty optional here for now, perhaps we can rework the ServerAdapter readiness path later on to incorporate a port

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I was also a little bit confused while reading the docs. I'll update the Adapter to the default behaviour in the interface

@aguibert
Copy link
Member

aguibert commented Jan 8, 2020

I use Eclipse as my editor, not sure if it honors .editorconfig or not. I might add a code formatting plugin which would format the code as part of the build (Quarkus uses this too)

@rieckpil
Copy link
Contributor Author

rieckpil commented Jan 9, 2020

I use Eclipse as my editor, not sure if it honors .editorconfig or not. I might add a code formatting plugin which would format the code as part of the build (Quarkus uses this too)

Unfortunately for Eclipse you would need a plugin (https://editorconfig.org/). If it can be automated within the build, then it's also fine 👍

Copy link
Member

@aguibert aguibert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks @rieckpil! I'll cut a new release in the next few days so that it becomes available on maven central.

@aguibert aguibert merged commit 4d5dc4c into MicroShed:master Jan 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
is:enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants